-
Notifications
You must be signed in to change notification settings - Fork 567
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
mkdeb.sh.in: pass remaining arguments to ./configure #5154
Conversation
Depends on #5142. |
Makefile.in
Outdated
DISTFILES_TEST = "test/Makefile.in test/apps test/apps-x11 test/apps-x11-xorg test/root test/private-lib test/fnetfilter test/fcopy test/environment test/profiles test/utils test/compile test/filters test/network test/fs test/sysutils test/chroot" | ||
DISTFILES = \ | ||
COPYING \ | ||
Makefile \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the dist
target is creating the tarball that is released after a new firejail version is released.
normally release tarballs don't contain generated files like Makefiles (or mkdeb.sh), as they likely can't be used by someone downloading the tarball (because they contain paths of the system where it was generated, or other configuration options).
(a common exception to that is the pre-generated configure
script, though in theory it also would not need to be shipped.)
i would prefer to keep a "clean" release tarball.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll look into this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the
dist
target is creating the tarball that is released after a new
firejail version is released. normally release tarballs don't contain
generated files like Makefiles (or mkdeb.sh), as they likely can't be used by
someone downloading the tarball (because they contain paths of the system
where it was generated, or other configuration options). (a common exception
to that is the pre-generatedconfigure
script, though in theory it also
would not need to be shipped.)i would prefer to keep a "clean" release tarball.
Yes, I don't consider it good either; I just didn't manage to think of
something better when opening this PR. As of the current version, this is not
done anymore.
Related to this, I now see that as of #5142, since the entirety of src/ and
test/ are part of DISTFILES
/ DISTFILES_TEST
(rather than only the exact
files and dirs that are needed), the generated Makefile
files in these
directories are also being copied on make dist
(besides just the
Makefile.in
files). But that should be fixed with the PR that will come
after #5140.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related to this, I now see that as of #5142, since the entirety of src/ and
test/ are part ofDISTFILES
/DISTFILES_TEST
(rather than only the exact
files and dirs that are needed), the generatedMakefile
files in these
directories are also being copied onmake dist
(besides just the
Makefile.in
files). But that should be fixed with the PR that will come
after #5140.
Actually, only src/ is affected, as test/Makefile.in is referenced directly in
DISTFILES_TEST
and there is no other makefile in test/.
Fixed by #5182.
Thanks for the CC. Having first-class support of custom configure options in deb package would be great! 👍 This PR doesn't build though? -
errors out with -
Oh, seems to manually require adding |
This pull request introduces 1 alert when merging c59ab8a into 880f2c9 - view on LGTM.com new alerts:
|
PACKAGE_TARNAME was added on commit 87e7b31 ("Configure Debian package with AA and SELinux options", 2020-05-13) / PR netblue30#3414. TOP was added on commit ed4a24c ("porting make deb-apparmor from LTS build", 2019-01-26).
This (mostly) restores the behavior from before commit 1fb814e ("Makefile.in: stop running distclean on dist", 2022-05-13) / PR netblue30#5142. ./configure still has to be called before calling ./contrib/fj-mkdeb.py (to generate Makefile from Makefile.in before calling `make distclean`).
@laniakea64 commented on May 24:
No problem.
Agreed; as of the current branch version, it should be more "first-class" than
Indeed, it looks like mkdeb.sh assumes I can try to make the script work with any prefix, but I'll probably only get |
Currently, mkdeb.sh (which is used to make a .deb package) runs ./configure with hardcoded options (some of which are automatically detected based on configure-time variables). To work around the hardcoding, contrib/fj-mkdeb.py is used to add additional options by rewriting the actual call to ./configure on mkdeb.sh. For example, the following invocation adds --disable-firetunnel to mkdeb.sh: $ ./configure && ./contrib/fj-mkdeb.py --disable-firetunnel To avoid depending on another script and to avoid re-generating mkdeb.sh, just let the latter pass the remaining arguments (the first one is an optional package filename suffix) to ./configure directly. Example: $ make distclean && ./configure && make dist && ./mkdeb.sh "" --disable-firetunnel Additionally, change contrib/fj-mkdeb.py to do roughly the same as the above example, by simply forwarding the arguments that it receives to ./mkdeb.sh (which then forwards them to ./configure). Also, remove the --only-fix-mkdeb option, since the script does not change mkdeb.sh anymore. With these changes, the script's usage (other than when using --only-fix-mkdeb) should remain the same. Note: To clean the generated files and then make a .deb package using the default configuration, the invocation is still the same: $ make distclean && ./configure && make deb Note2: Running ./configure in the above examples is only needed for generating Makefile/mkdeb.sh from Makefile.in/mkdeb.sh.in after running distclean, so that running `make` / `./mkdeb.sh` afterwards works. Should fully fix netblue30#772. Relates to netblue30#1205 netblue30#3414 netblue30#5148.
Since `make deb-apparmor` already exists, use that for now instead of changing what `make deb` does. This fixes CI. Added on commit 494b26d ("adding --enable-apparmor by default for make deb - most Debian-based distros have apparmor enabled by default", 2022-06-03). Kind of relates to netblue30#5154.
This reverts commit 1fb814e. If distclean is not executed before copying the files on dist, then the generated files inside src/ are included in the dist archive: $ ./configure >/dev/null && make distclean >/dev/null && ./configure >/dev/null && make dist | grep 'Makefile$' | wc -l 26 This happens because src/ is copied wholesale on dist (see DISTFILES). Revert the commit to ensure that only the input files (such as the "Makefile.in" files) are archived. Related discussion: netblue30#5154 (review) Relates to netblue30#5142.
Sorry I couldn't get back to this sooner.
That error can be worked around by running
The first-class method to build .deb with custom configure options works and does not have these problems. Also it is straightforward enough that
Thanks but I think it's not necessary. The only time I've ever needed an alternate prefix .deb was when I had two versions of firejail installed concurrently (the reason is documented on this issue tracker somewhere and is no longer relevant), and I had to change more than just prefix to make that work. |
This reverts commit b4d0b24. This amends commit 56b86f8 ("Revert "Makefile.in: stop running distclean on dist"", 2022-06-08) / PR #5182. Since the revert, `make dist` itself already runs `make distclean`. This also means that it is no longer necessary to run ./configure (to generate "Makefile" from "Makefile.in") before running ./contrib/fj-mkdeb.py. Misc: This is not a clean revert. Relates to #5154.
Instead of using the first argument as the `EXTRA_VERSION` variable. This should make the usage of mkdeb.sh less confusing, especially when one is not trying to set the variable. As for using `EXTRA_VERSION` (which is still optional with this commit), make sure that it is set as an environment variable before caling mkdeb.sh. Example: env EXTRA_VERSION=-apparmor ./mkdeb.sh --enable-apparmor See also commit 9a0fbbd ("mkdeb.sh.in: pass remaining arguments to ./configure", 2022-05-13) / PR netblue30#5154.
@laniakea64 commented on Jun 9, 2022:
No worries; likewise.
That is because as of this PR (#5154), the first argument to mkdeb.sh (and thus ./contrib/fj-mkdeb.py '' --disable-firetunnel --enable-apparmor And as of #5654, that is no longer the case; it should be possible to just pass ./contrib/fj-mkdeb.py --disable-firetunnel --enable-apparmor
For the most part, yes; fj-mkdeb.py seems to boil down to the following: #!/bin/sh
set -e
make distclean
./configure "$@" --prefix=/usr
make dist
./mkdeb.sh "$@"
Possibly; not sure if anyone else is using it. I might open a PR to remove it |
The "deb" target depends on the "dist" target, which creates an archive from DISTFILES. The arguments to ./configure are misleading, as they do not affect the archive that is used by `make deb`. That is the case because the configure output files (config.mk and config.sh) are not copied into the dist archive, only their input files (config.mk.in and config.sh.in). In order to affect the .deb package, the configure arguments have to be passed to mkdeb.sh, which then forwards them to ./configure itself. Note: This does not apply to the rpm-based jobs, as `make rpms` uses the files directly rather than using the dist archive. Relates to netblue30#5154.
The official .deb package is always built with apparmor support, so use `--enable-apparmor` in mkdeb.sh and remove the "deb-apparmor" target in order to reduce redundancy. Note that custom configure options may be specified by calling ./mkdeb.sh directly. For example, to build the .deb package without apparmor support, instead of running `make deb`, the following commands can be used: make dist ./mkdeb.sh --disable-apparmor Also, change the `build_apparmor` GitLab CI job into `build_no_apparmor`, which is intended to check that building without apparmor still works. Note: This commit makes the resulting .deb package not have an "-apparmor" suffix (see `EXTRA_VERSION` in mkdeb.sh), to avoid redundancy (as having apparmor support becomes the default). Misc: This is a follow-up to netblue30#5654. Relates to netblue30#5154 netblue30#5176 netblue30#5547.
For consistency with mkdeb.sh. Note: The default arguments and support for argument overriding was added to to mkrpm.sh on commit 3d97332 ("Add configure options when building rpm (netblue30#3422)", 2020-05-19). The support for appending arguments was added to mkdeb.sh on commit 9a0fbbd ("mkdeb.sh.in: pass remaining arguments to ./configure", 2022-05-13) / PR netblue30#5154.
Currently, mkdeb.sh (which is used to make a .deb package) runs
./configure with hardcoded options (some of which are automatically
detected based on configure-time variables). To work around the
hardcoding, contrib/fj-mkdeb.py is used to add additional options by
rewriting the actual call to ./configure on mkdeb.sh. For example, the
following invocation adds --disable-firetunnel to mkdeb.sh:
To avoid depending on another script and to avoid re-generating
mkdeb.sh, just let the latter pass the remaining arguments (the first
one is an optional package filename suffix) to ./configure directly.
Example:
Additionally, change contrib/fj-mkdeb.py to do roughly the same as the
above example, by simply forwarding the arguments that it receives to
./mkdeb.sh (which then forwards them to ./configure). Also, remove the
--only-fix-mkdeb option, since the script does not change mkdeb.sh
anymore. With these changes, the script's usage (other than when using
--only-fix-mkdeb) should remain the same.
Note: To clean the generated files and then make a .deb package using
the default configuration, the invocation is still the same:
Note2: Running ./configure in the above examples is only needed for
generating Makefile/mkdeb.sh from Makefile.in/mkdeb.sh.in after running
distclean, so that running
make
/./mkdeb.sh
afterwards works.Should fully fix #772.
Relates to #1205 #3414 #5142 #5148.